ATM: Simplify query configurations #11323

tiferet · 2022-11-18T01:03:14Z

This PR simplifies the query configurations. It moves common functionality from the derived classes up to AtmConfig. It also combines the two configs each query previously had into a single config, simplifying the code and making it easier to understand. This will prevent code duplication and bugs, improve maintainability, and make it easier to add new queries.

Once again commit-by-commit review is recommended 😄

Main changes

AtmConfig inherits from TaintTracking::Configuration. That way the specific query configs (e.g. DomBasedXssAtmConfig), which inherit from AtmConfig, also inherit from TaintTracking::Configuration. This removes the need for two separate config classes for each query (e.g. DomBasedXssAtmConfig and XssATM::Configuration). Note: We could also keep AtmConfig as is and have the query configs inherit both from it and from TaintTracking::Configuration directly, but I think having AtmConfig inherit from TaintTracking::Configuration makes more sense: it's a query configuration for a boosted taint tracking query, which is a type of taint tracking query that adds a few extra predicates (e.g. isKnownSink).
Move the definition of isSource to the base class: A long as we're not boosting sources, isSource is identical to isKnownSource.
Move the definition of isSink to the base class: Holds if sink is a known taint sink or an "effective" sink (a candidate to be classified by an ML model).
Remove code duplication in query .ql files: Define the query for finding ATM alerts in the base class AtmConfig, and call it from each query's .ql file.

Consistency check

This DCA experiment uses the model on main with the final commit of each PR from the CodeQL revamp. The idea is to double-check that none of the changes I've made has affected the endpoints that get scored at inference time, since this is not directly tested for in the PR checks. We see identical results from all variants (e.g. this), confirming that none of the revamp work has changed the endpoints being scored.

Timing checks

✅ KPI timing experiment: https://github.com/github/codeql-dca-main/issues/8684
☑️ The local runtime of endpoint_large_scale/ExtractEndpointDataTraining remains like it was after the last PR that affected timing: About 5s.

Addresses most of https://github.com/github/ml-ql-adaptive-threat-modeling/issues/2134.

...imental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll

tiferet · 2022-11-22T18:19:17Z

@kaeluka when you review this PR, do you mind looking at the possible issue Henry raised here. I think it's OK, but I have limited understanding of the issue he's pointing out. Henry said From a very very quick scan, it looks OK, but I’d recommend taking a closer look. I'd appreciate your eyes on it as well to make sure there isn't a problem.

kaeluka · 2022-11-24T13:30:58Z

Thank you for point this out! I was going to, but I definitely want to talk to Henry about it as well!

kaeluka · 2022-11-28T13:42:24Z

As part of digging into the questions brought up by Henry, I added the following snippet to each file modified in this PR and quick-evaluated it:

predicate test(DataFlow::Configuration a) {
  any()
}

This showed me that there's actually a few files where there's several instances of DataFlow::Configuration in scope. I believe that, in each case, the situation is not preventing the PR from being merged.

These files are:

✅ EndpointFeatures.ql - benign because the query only computes features, rather than evaluating flow.
✅ FilteredTruePositives.ql - benign because the query only runs queries involving characteristics, rather than evaluating flow (and it's just a unit test).
✅ ExtractEndpointMapping.ql - benign because the query does not evaluate flow
✅ DebugResultInclusion.ql - this might actually be slowed down because it uses hasFlow but a) it's only a debug query, and b) the situation is like that already on main (the PR only changes some names in the file).

@henrymercer, is my reasoning sound?

henrymercer · 2022-11-28T15:13:27Z

@henrymercer, is my reasoning sound?

Your reasoning around the files you listed looks good. Most of those files are test or debug files. I'm not sure how you handled the case of libraries being changed, as we'd need to check all the files that use this library and not just the ones being changed.

I did try evaluating the predicate top-down on each of the queries and they each reported a single dataflow configuration, so I think we're good to go.

kaeluka · 2022-11-28T15:48:49Z

Perfect! I'll continue with the conventional part of the review :) This should be done ~tomorrow lunchtime, @tiferet. Only minor comments so far, but will take some time to think about the big picture tomorrow.

tiferet · 2022-11-28T19:42:08Z

@kaeluka Since you're already in the middle of reviewing this PR, I don't want to rebase it on top of the latest version of tiferet/complexity-reduction and interfere with your ongoing work. Please LMK what your preference is. Should I:

Rebase on top of tiferet/complexity-reduction now?
Wait until tiferet/complexity-reduction is merged to main and then rebase on top of main?
Wait until tiferet/complexity-reduction is merged to main and then merge main into this branch?
Something else?

kaeluka

The big question, whether this might introduce a pathological dependency graph, here has been tackled in the PR discussion already.

👍: The rest of the review is that I:

Like the change and am not asking you to make any big changes.
I'd slightly prefer to follow convention in the naming of the Config classes but it's not a hill I'll die on.
I was a bit surprised to see the PR, as it seems like it's a bit of a side track from the grand design we're following. Happy to see it either way, though!

...ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll

...imental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll

...erimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll

javascript/ql/experimental/adaptivethreatmodeling/src/NosqlInjectionATM.ql

javascript/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/EndpointFeatures.ql

All remaining functionality in `CoreKnowledge` is only being used in `EndpointCharacteristics`, so it can be moved there as a small set of helper predicates.

All remaining functionality in `StandardEndpointFilters` is only being used in `EndpointCharacteristics`, so it can be moved there as a small set of helper predicates.

That way the specific configs which inherit from `AtmConfig` also inherit from `TaintTracking::Configuration`. This removes the need for two separate config classes for each query.

A long as we're not boosting sources, `isSource` is identical to `isKnownSource`.

Holds if `sink` is a known taint sink or an "effective" sink.

Define the query for finding ATM alerts in the base class `AtmConfig`, and call it from each query's .ql file.

Name the query configuration e.g. `NosqlInjectionATMConfig` rather than `Configuration`.

tiferet · 2022-11-29T23:52:39Z

I was a bit surprised to see the PR, as it seems like it's a bit of a side track from the grand design we're following. Happy to see it either way, though!

Yes, I think we're already done implementing the design. All the issues left here are improvements we can make either to the code or to the data now that the refactoring is complete 🎉

adityasharad · 2022-11-29T23:52:11Z

...ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll

+   * to this ML-boosted configuration, whereas the unboosted base query does not contain this source and sink
+   * combination.
+   */
+  predicate getAlerts(JS::DataFlow::PathNode source, JS::DataFlow::PathNode sink, float score) {


Minor/bikeshed: I'd prefer a name that doesn't suggest a return/result value, and mentions flow. Also makes it easier to see the parallels between unboosted and boosted queries. How about hasLikelyFlowPath?

I don't know about hasLikelyFlowPath, because one of the conditions it must meet is hasFlowPath. Logically hasFlowPath should imply hasLikelyFlowPath.

Maybe something like hasBoostedFlowPath? We've been trying to get rid of the "boosted" terminology, but I can't think of something better right now. I think hasAlert would be clearer (ultimately that's what this predicate does -- it surfaces all the alerts for this config), but that doesn't mention flow.

Note: I tracked this renaming together with the other renamings here, so that we can merge this PR and XssThroughDOM and open a separate PR for all the renamings

github-actions bot added the ATM label Nov 18, 2022

tiferet commented Nov 18, 2022

View reviewed changes

...imental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll Show resolved Hide resolved

tiferet marked this pull request as ready for review November 18, 2022 20:09

tiferet requested review from a team and kaeluka and removed request for a team November 18, 2022 20:09

tiferet force-pushed the tiferet/complexity-reduction branch from e6c76a5 to 9a37061 Compare November 19, 2022 00:08

tiferet force-pushed the tiferet/simplify-configs branch from a543d95 to d8ef142 Compare November 19, 2022 00:10

tiferet force-pushed the tiferet/complexity-reduction branch from 9a37061 to fac6641 Compare November 21, 2022 16:34

tiferet force-pushed the tiferet/simplify-configs branch from d8ef142 to 42cc1bc Compare November 21, 2022 16:46

owen-mc changed the title ~~Simplify query configurations~~ ATM: Simplify query configurations Nov 22, 2022

kaeluka mentioned this pull request Nov 22, 2022

ATM: Implement the current endpoint filters as EndpointCharacteristics #11281

Merged

tiferet force-pushed the tiferet/complexity-reduction branch from fac6641 to 4580b55 Compare November 28, 2022 19:36

kaeluka previously approved these changes Nov 29, 2022

View reviewed changes

Base automatically changed from tiferet/complexity-reduction to main November 29, 2022 21:17

tiferet dismissed kaeluka’s stale review via 6813358 November 29, 2022 21:17

tiferet added 7 commits November 29, 2022 13:20

Delete CoreKnowledge.

5402f04

All remaining functionality in `CoreKnowledge` is only being used in `EndpointCharacteristics`, so it can be moved there as a small set of helper predicates.

Delete StandardEndpointFilters.

05a943c

All remaining functionality in `StandardEndpointFilters` is only being used in `EndpointCharacteristics`, so it can be moved there as a small set of helper predicates.

AtmConfig inherits from TaintTracking::Configuration.

50291c7

That way the specific configs which inherit from `AtmConfig` also inherit from `TaintTracking::Configuration`. This removes the need for two separate config classes for each query.

Move the definition of isSource to the base class:

cd24ec8

A long as we're not boosting sources, `isSource` is identical to `isKnownSource`.

Move the definition of isSink to the base class:

a710b72

Holds if `sink` is a known taint sink or an "effective" sink.

Remove code duplication in query .ql files:

75cd7a9

Define the query for finding ATM alerts in the base class `AtmConfig`, and call it from each query's .ql file.

Doc suggestion from code review

6f807e9

tiferet force-pushed the tiferet/simplify-configs branch from 6813358 to 6f807e9 Compare November 29, 2022 21:22

Suggestion from code review:

c5184d3

Name the query configuration e.g. `NosqlInjectionATMConfig` rather than `Configuration`.

tiferet requested a review from kaeluka November 29, 2022 23:49

adityasharad reviewed Nov 29, 2022

View reviewed changes

tiferet mentioned this pull request Nov 30, 2022

ATM: Boost XssThroughDOM #11486

Merged

adityasharad approved these changes Nov 30, 2022

View reviewed changes

tiferet merged commit e2e3667 into main Nov 30, 2022

tiferet deleted the tiferet/simplify-configs branch November 30, 2022 01:39

ATM: Simplify query configurations #11323

ATM: Simplify query configurations #11323

Uh oh!

Conversation

tiferet commented Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Main changes

Consistency check

Timing checks

Uh oh!

Uh oh!

tiferet commented Nov 22, 2022

Uh oh!

kaeluka commented Nov 24, 2022

Uh oh!

kaeluka commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henrymercer commented Nov 28, 2022

Uh oh!

kaeluka commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiferet commented Nov 28, 2022

Uh oh!

kaeluka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tiferet commented Nov 29, 2022

Uh oh!

adityasharad Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

tiferet Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

tiferet Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tiferet commented Nov 18, 2022 •

edited

Loading

kaeluka commented Nov 28, 2022 •

edited

Loading

kaeluka commented Nov 28, 2022 •

edited

Loading